Skip to content

correct test cases to properly check result #826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 27, 2017
Merged

correct test cases to properly check result #826

merged 1 commit into from
Oct 27, 2017

Conversation

felixdoerre
Copy link
Contributor

When working with "file_line" and "ensure=>absent", I noticed that my existing manifests that were perfectly fine in 4.18.0, stopped working in 4.19.0. Digging deeper into the issue, I was able to identify commit 81d7d35 that caused the problem. In order to provide a minimal working example to describe the issue, I stumbled over the example from the "README.md":

file_line { 'bashrc_proxy':
  ensure             => present,
  path               => '/etc/bashrc',
  line               => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128',
  match              => '^export\ HTTP_PROXY\=',
  append_on_no_match => false,
}

I found out, that this example only works when the line to remove is exactly "export HTTP_PROXY=http://squid.puppetlabs.vm:3128" and does not do anything for a line like "export HTTP_PROXY=a".

I checked and saw that these test cases are exactly the ones run by "spec/unit/puppet/provider/file_line/ruby_spec.rb" which surprised me and I wondered how theses testcases could pass.

So after more debugging I saw that the problematic test cases don't even create the provider. When I corrected that, I noticed that the testcases were sill passing. I realized that the test cases don't check the "exists?" method, which is used when applying manifests with such resources. So I added that assertion as well.

Note that the corrected test cases will not pass, as the code from commit 81d7d35 causes such file_line resources to be handled incorrectly, which I did not attempt to correct.

@felixdoerre felixdoerre force-pushed the fix-file_line-testcases branch from 7038db5 to 10c26c1 Compare October 14, 2017 12:13
@felixdoerre
Copy link
Contributor Author

When poking around in this issue and trying to solve the problem at the initial implementation I discovered two things:
a) The code indicates that adding "replace => false" will have the same effect. Interestingly this is also indicated in a comment for the stackoverflow-question that caused this test cases to be added.
b) I tried to add the behavior for "match_for_absent" back in, that seems to be missing in the current implementation of the "ruby" provider. When comparing it with != 'true' the code seems to fulfill all test cases. However == 'false' does not seem to work. I am not sure how default values are handled here, and therefore not 100% sure that my proposed commit is correct.

@felixdoerre
Copy link
Contributor Author

It seems that #825 has completely rewritten the "exists?" from file_line and on-the-fly been fixing this problem. So that leaves me to only correct the test cases. I will update the branch accordingly.

@felixdoerre felixdoerre force-pushed the fix-file_line-testcases branch from 10c26c1 to 33bcee7 Compare October 19, 2017 10:28
@felixdoerre
Copy link
Contributor Author

And just in case someone did not already see the newer push from 5 days ago: I pushed an update to the branch so it only corrects the testcases. Can someone have a look at it?

@david22swan
Copy link
Member

david22swan commented Oct 26, 2017 •
edited
Loading

The provider is being created by the before statement.
It is run before each it and sets up the resources for the example.
You can see it at line 626 in the file.

In addition to this the 'exists?' is being called within the first iteration of the it within the overall context, with the expectation being that if it exists within the first one, it will exist within all the remaining ones due to the nature of the before statement.

Given this am unsure if your commit is necessary or adds anything to the code.

I apologize if I have been unclear in any manner and thank you for taking the time to make additions to the module.

@felixdoerre
Copy link
Contributor Author

Thanks for the comment. Yes, I saw that a provider is created in line 626. However the test 655 wants to test a different resource (created in 656). Therefore I assumed that the provides has to be recreated. The same Pattern is already used in line 320 and then overridden in line 342, so I guessed that the same pattern needs to be used here. Am I wrong?

@felixdoerre
Copy link
Contributor Author

felixdoerre commented Oct 26, 2017 •
edited
Loading

I am not quite sure that I fully understand your point. I am sure that my changes are necessary, as the test cases previously failed to detect the regression introduced by 81d7d35 (that caused manifests on my server to not be applied correctly). Directly concerning your comment:
Yes, "exists?" is tested in line 644, however all following test cases replace the resource "under test" with a testcase specific one. (lines 656, 674, and more). You can see that I only touched testcases ("it"-blocks) where the resource defined by the context is specifically overridden. (e.g. for "it"-block 655 the 'generic' resource from 627 is overridden with a testcase-specific one in line 656)

@david22swan
Copy link
Member

Ah, I see your point know.
That'll teach me to review PRs last thing before I leave at night.

@david22swan
Copy link
Member

My apologies

@david22swan david22swan merged commit b691910 into puppetlabs:master Oct 27, 2017
@felixdoerre
Copy link
Contributor Author

:-) thanks for doing review and thanks for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixdoerre @david22swan @jordanbreen28